Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Logi Circle component, camera and sensor platform #16540

Merged
merged 7 commits into from
Sep 21, 2018

Conversation

evanjd
Copy link
Contributor

@evanjd evanjd commented Sep 10, 2018

Description:

This adds the logi_circle component, camera and sensor platforms for Logi Circle 1st and 2nd gen cameras.

Screenshot of working configuration (2x Logi Cameras shown)
screen shot 2018-09-13 at 8 40 44 pm

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#6249

Example entry for configuration.yaml (if applicable):

logi_circle:
  username: YOUR_USERNAME
  password: YOUR_PASSWORD

camera:
  - platform: logi_circle

sensor:
  - platform: logi_circle

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

…s not supported by device. Added services for recording livestream and taking a snapshot from the livestream.
…assistant components and into the logi circle API wrapper. Added services.yaml entries for logi services.
…e` and tidy up in preparation for pull request.

- Renamed `logi_set_mode` to `logi_set_config`.
- Live stream recording and snapshot methods now respect whitelisted path configuration.
- Added `streaming_mode` and `speaker_volume` sensors.
- Moved model-specific turn on/off logic to `logi_circle` library.
@homeassistant
Copy link
Contributor

Hi @evanjd,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

asyncio.shield(self._camera.record_livestream(
stream_file, timedelta(seconds=duration)), loop=self.hass.loop)

async def livestream_snapshot(self, filename):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick explanation on why this was implemented when there's already a generic snapshot service for cameras:

When not live streaming, Logi cameras (particularly battery-powered devices) go into a low power state, only taking a snapshot every 1-5 minutes. This cached image is what the async_camera_image method uses.

This method exists if you really need to wake the camera from sleep immediately to take a snapshot. I didn't want this to be the default source for camera stills as it increases battery drain.

@balloob
Copy link
Member

balloob commented Sep 13, 2018

Can you rename the component to logi_circle

@evanjd
Copy link
Contributor Author

evanjd commented Sep 13, 2018

@balloob Done.


from homeassistant.helpers import config_validation as cv
from homeassistant.components.logi_circle import (
DOMAIN, CONF_ATTRIBUTION)
Copy link
Member

@MartinHjelmare MartinHjelmare Sep 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This platform belongs to the camera domain. Import the logi_circle domain as another name.

from homeassistant.components.logi_circle import (
    DOMAIN as LOGI_CIRCLE_DOMAIN, ...

DOMAIN, CONF_ATTRIBUTION)
from homeassistant.components.camera import (
Camera, PLATFORM_SCHEMA, CAMERA_SERVICE_SCHEMA, SUPPORT_ON_OFF,
ATTR_ENTITY_ID, ATTR_FILENAME, DOMAIN as CAMERA_DOMAIN)
Copy link
Member

@MartinHjelmare MartinHjelmare Sep 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This domain should be imported as domain. 😉

})


async def async_setup_platform(hass,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please break the line after the first parenthesis and then fill out as far as possible.


def __init__(self, hass, camera, device_info):
"""Initialize Logi Circle camera."""
super(LogiCam, self).__init__()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super().__init__()

'firmware': self._camera.firmware,
'generation': self._camera.model_generation,
'ip_address': self._camera.ip_address,
'mac_address': self._camera.mac_address,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self._icon = 'mdi:{}'.format(SENSOR_TYPES.get(self._sensor_type)[2])
self._name = "{0} {1}".format(
self._camera.name, SENSOR_TYPES.get(self._sensor_type)[0])
self._state = STATE_UNKNOWN
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use None to represent unknown state.

ATTR_ATTRIBUTION: CONF_ATTRIBUTION,
'battery_saving_mode': ('on' if self._camera.battery_saving
else 'off'),
'firmware': self._camera.firmware,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above about appropriate state attributes.

'firmware': self._camera.firmware,
'generation': self._camera.model_generation,
'ip_address': self._camera.ip_address,
'mac_address': self._camera.mac_address,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Define unique id property with this.

def icon(self):
"""Icon to use in the frontend, if any."""
if (self._sensor_type == 'battery_level' and
self._state != STATE_UNKNOWN):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use STATE_UNKNOWN.

else:
state = getattr(self._camera, self._sensor_type, STATE_UNKNOWN)
if isinstance(state, bool):
self._state = 'on' if state is True else 'off'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use STATE_ON and STATE_OFF from const.py. But probably we should instead make a binary sensor platform for that type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two binary sensors the camera exposes through the sensor platform are:

  • streaming mode: determines whether the camera can return video/stills and whether it will record activities (motion, etc)
  • privacy mode: determines whether the camera will record activities if they're detected

Neither of these seem to fit neatly into any existing binary sensor class, which is why I opted to make them sensors instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can define binary sensors without a specific device class. If a sensor only has a binary state, it belongs with the binary sensor platform.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, my bad. As you advised, I'll migrate these sensors to a binary sensor in my next PR.

* Added timeout of 15s to logi_circle initial setup requests (login and grabbing cameras).
* Added unique ID (uses MAC address for camera platform, MAC address + sensor type for sensor platform).
* Added battery level and battery charging attributes to camera.
* Removed static attributes from device_state_attributes.
* Replaced STATE_UNKNOWN with None, replaced ‘on’ & ‘off’ with STATE_ON and STATE_OFF.
* Removed redundant SCAN_INTERVAL in sensor, removed redundant hass param from async_setup_platform in camera and sensor.
* Style tweaks.
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

I would suggest moving the sensors that have binary states to the binary_sensor platform. That can happen in another PR though.

@evanjd
Copy link
Contributor Author

evanjd commented Sep 18, 2018

@MartinHjelmare Thanks for the review!

@balloob balloob merged commit aeaf694 into home-assistant:dev Sep 21, 2018
@ghost ghost removed the in progress label Sep 21, 2018
@balloob balloob mentioned this pull request Sep 28, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Feb 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants